[24.1] Fix empty usernames in database + bug in username generation #18379
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The packages error is an import error ( |
|
I have dropped the unit test for now (it passed) - I'll add it in a separate PR targeting dev. For this to be testable from within the data package, the test should not import from managers. In dev, this is easily solvable by moving db-specific functions into the new |
mvdbeek
reviewed
Jun 12, 2024
| update, | ||
| ) | ||
|
|
||
| from galaxy.managers.users import username_from_email_with_connection |
There was a problem hiding this comment.
Can't import from managers here, that breaks the package structure. Generally speaking it's probably wise to have a fixed copy in the migration so we don't alter migration results accidentally.
Otherwise revision details are displayed in list output: INCORRECT: 92fb564c7095 -> ddbdbc40bdc1 (gxy), add workflow_comment table Revision ID: ddbdbc40bdc1 Revises: 92fb564c7095 Create Date: 2023-08-14 13:41:59.442243 987ce9839ecb, e93c5d0b47a9 -> 92fb564c7095 (gxy) (mergepoint), Merge alembic heads after pulling external_user_id length extension forward EXPECTED: 92fb564c7095 -> ddbdbc40bdc1 (gxy), add workflow_comment table 987ce9839ecb, e93c5d0b47a9 -> 92fb564c7095 (gxy) (mergepoint), Merge alembic heads after pulling external_user_id length extension forward
The bug was introduced when moving this code to SQLAlchemy 2.0 The statement in generate_next_available_username should be evaluated at each step of the iteration: only then will the new value of `i` result in a new statement.
jdavcs
force-pushed
the
241_fix_usernames_in_db
branch
from
7f4c0f8 to
51309da
Compare
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a follow-up to #18364 (ref #18364 (comment)).
I've testing this locally (the CI will run the migration too)
This also fixes a newly-discovered bug introduced in #16724 during migrating db code to SA2.0 (see commit description).
I'll backport the bug fix to 23.2. I'm not targeting this at 23.2 because this has tests that rely on data access testing infrastructure introduced in 24.1.
Question: do we want to apply the same type of fix to the tool_shed database schema and data? If so, I'll add a commit with a TS migration.
How to test the changes?
(Select all options that apply)
License